Remap paths in UI tests by default #105924

TimNN · 2022-12-19T21:49:24Z

If you think this needs further discussions / something RFC-like, please let me know the best forum for that.

This PR runs UI tests with a remapped "src base" directory by default.

Why? Because some UI tests currently depend on the length of the absolute path to the src/test/ui directory. Remapping makes the tests independent of the absolute path.

The path to the source file (which is absolute on CI) is part of the type name of closures. rustc diagnostic output depends on the length of type names (long type names are truncated). So a long absolute path leads to long closure type names, which leads to truncation and changed diagnostics.

(I initially tried just disabling type name truncation, but that made some error messages stupid long (thousands of characters, IIRC)).

Additional changes:

All boolean compiletest directives now support explicit no- versions to disable them.
Adapt existing tests when necessary:
- Disable remapping for individual tests that fail with it enabled (when there's no obvious alternative fix).
- For tests that already check something remapping related switch to the new option unless we gain something significant by keeping the manual remap.

Passed Windows CI in https://github.com/rust-lang/rust/actions/runs/3933100590

ChrisDenton · 2023-01-06T11:07:50Z

(edit: Though I see now that Windows seems to only run for auto so I guess I'll have to figure something else out for that).

See the Using CI to test section of the dev guide for how to test Windows jobs in a PR.

TimNN · 2023-01-06T11:17:52Z

@ChrisDenton: Thanks for pointing out the docs! I though something like that would be possible, but didn't realize it was officially documented. (I just got back to this PR and finished setting up a windows VM...)

TimNN · 2023-01-06T13:51:55Z

r? bootstrap

jyn514 · 2023-01-10T00:09:00Z

r? @Mark-Simulacrum

(#106657)

TimNN · 2023-01-13T15:26:10Z

I've been considering this a bit more and will be moving the first two commits to separate PRs (rental & path mapping undo).

Update `rental` hack to work with remapped paths. This PR simply switches to an already-existing helper instead of hard-coding a specific enum variant. The new revision of the test fails without the other changes in this PR. Context: I'm exploring running UI tests with remapped paths by default in rust-lang#105924 and the rental test was one of the ones that failed. This may also be useful in the context of rust-lang/rfcs#3127 ("New rustc and Cargo options to allow path sanitisation by default").

Mark-Simulacrum · 2023-01-14T02:02:10Z

@rustbot author

Update `rental` hack to work with remapped paths. This PR simply switches to an already-existing helper instead of hard-coding a specific enum variant. The new revision of the test fails without the other changes in this PR. Context: I'm exploring running UI tests with remapped paths by default in rust-lang#105924 and the rental test was one of the ones that failed. This may also be useful in the context of rust-lang/rfcs#3127 ("New rustc and Cargo options to allow path sanitisation by default").

Heuristically undo path prefix mappings. Because the compiler produces better diagnostics if it can find the source of (potentially remapped) dependencies. The new test fails without the other changes in this PR. Let me know if you have better suggestions for the test directory. I moved the existing remapping test to be in the same location as the new one. Some more context: I'm exploring running UI tests with remapped paths by default in rust-lang#105924 and this was one of the issues discovered. This may also be useful in the context of rust-lang/rfcs#3127 ("New rustc and Cargo options to allow path sanitisation by default").

TimNN · 2023-01-16T18:39:05Z

Rebased on latest master, on top of the split-out rental and path mapping changes.
Updated more test cases to use the new option instead of manual remapping.
Updated the PR description in the first comment.
Temporarily added the Windows CI again.

Should generally be ready for review, I'll ping the bot to update the labels once Windows CI is green.

TimNN · 2023-01-16T20:01:26Z

@rustbot ready

Mark-Simulacrum · 2023-01-21T16:07:03Z

So a long absolute path leads to long closure type names, which leads to truncation and changed diagnostics.

Hm, I thought that we had a separate filtering pass which made this not true - #96551?

Or is the truncation happening within rustc?

TimNN · 2023-01-21T16:09:49Z

The truncation is happening in rustc. It shows a very abbreviated type name and includes another note stating something along the lines of "the full type name has been written to [some tmp path]".

TimNN · 2023-01-21T16:11:58Z

Here is an example diff we see if the filename is too long:

---- [ui] rust-llvm-integrate-prototype/tests/ui/never_type/fallback-closure-wrap.rs#fallback stdout ----

diff of stderr:

10	   |
11	   = note: expected unit type `()`
12	                   found type `!`
-	   = note: required for the cast from `[closure@$DIR/fallback-closure-wrap.rs:18:40: 18:47]` to the object type `dyn FnMut()`
+	   = note: required for the cast from `[closure@fallback-closure-wrap.rs:18:40]` to the object type `dyn FnMut()`
+	   = note: the full name for the casted type has been written to '$TEST_BUILD_DIR/never_type/fallback-closure-wrap.fallback/fallback-closure-wrap.long-type-6621918892693463368.txt'
14	
15	error: aborting due to previous error
16

edit: not sure if this going to work, but let's try to avoid spamming people: @rustbot ready

Mark-Simulacrum · 2023-01-21T16:31:47Z

Huh! This is news to me, maybe a recent change.

In any case, this seems like a reasonable approach to solving that problem -- @bors r+ rollup=iffy

bors · 2023-01-21T16:31:49Z

📌 Commit cd1d0bc has been approved by Mark-Simulacrum

It is now in the queue for this repository.

bors · 2023-01-21T17:44:23Z

⌛ Testing commit cd1d0bc with merge 52372f9...

bors · 2023-01-21T20:56:47Z

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 52372f9 to master...

rust-timer · 2023-01-21T23:20:03Z

Finished benchmarking commit (52372f9): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.3%	[-0.3%, -0.3%]	2
Improvements ✅ (secondary)	-0.6%	[-0.8%, -0.3%]	7
All ❌✅ (primary)	-0.3%	[-0.3%, -0.3%]	2

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-5.1%	[-5.1%, -5.1%]	1
All ❌✅ (primary)	-	-	0

Cycles

This benchmark run did not return any relevant results for this metric.

Heuristically undo path prefix mappings. Because the compiler produces better diagnostics if it can find the source of (potentially remapped) dependencies. The new test fails without the other changes in this PR. Let me know if you have better suggestions for the test directory. I moved the existing remapping test to be in the same location as the new one. Some more context: I'm exploring running UI tests with remapped paths by default in rust-lang/rust#105924 and this was one of the issues discovered. This may also be useful in the context of rust-lang/rfcs#3127 ("New rustc and Cargo options to allow path sanitisation by default").

rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 19, 2022

TimNN force-pushed the ui-remap branch 2 times, most recently from 7ec33cb to ba526ed Compare December 20, 2022 11:46

TimNN force-pushed the ui-remap branch from ba526ed to c180202 Compare January 6, 2023 10:34

TimNN force-pushed the ui-remap branch from c180202 to d0b3342 Compare January 6, 2023 11:50

rustbot added the T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. label Jan 6, 2023

TimNN force-pushed the ui-remap branch 3 times, most recently from c7ec808 to 1f9a3d3 Compare January 6, 2023 13:46

TimNN changed the title ~~WIP: Remap paths in UI tests by default~~ Remap paths in UI tests by default (and reverse path mappings for diagnostics where possible) Jan 6, 2023

TimNN changed the title ~~Remap paths in UI tests by default (and reverse path mappings for diagnostics where possible)~~ Remap paths in UI tests by default Jan 6, 2023

rustbot assigned jyn514 Jan 6, 2023

rustbot assigned Mark-Simulacrum and unassigned jyn514 Jan 10, 2023

TimNN force-pushed the ui-remap branch from 1f9a3d3 to bd6cef4 Compare January 13, 2023 15:42

TimNN mentioned this pull request Jan 13, 2023

Update rental hack to work with remapped paths. #106816

Merged

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 14, 2023

TimNN mentioned this pull request Jan 14, 2023

Heuristically undo path prefix mappings. #106853

Merged

ui tests: Remap test base directory by default.

cd1d0bc

TimNN force-pushed the ui-remap branch from bd6cef4 to 463ad0f Compare January 16, 2023 18:35

TimNN force-pushed the ui-remap branch from 463ad0f to cd1d0bc Compare January 16, 2023 20:01

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 16, 2023

Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 21, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 21, 2023

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 21, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 21, 2023

bors merged commit 52372f9 into rust-lang:master Jan 21, 2023

rustbot added this to the 1.68.0 milestone Jan 21, 2023

TimNN deleted the ui-remap branch January 21, 2023 20:58

jyn514 mentioned this pull request Apr 3, 2023

ui test error output doesn't play nicely with "goto file" in vscode #109725

Closed

jyn514 mentioned this pull request Apr 27, 2023

Make method-not-found-generic-arg-elision.rs error message not path dependent #110866

Merged

TimNN mentioned this pull request Jun 3, 2023

1.70.0: ui test failures due to "the full type name has been written to" #112209

Closed

mdibaiee mentioned this pull request Jul 25, 2023

"the full type name has been written to disk" causes UI tests to fail #113424

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remap paths in UI tests by default #105924

Remap paths in UI tests by default #105924

TimNN commented Dec 19, 2022 •

edited

Loading

ChrisDenton commented Jan 6, 2023

TimNN commented Jan 6, 2023

TimNN commented Jan 6, 2023

jyn514 commented Jan 10, 2023 •

edited

Loading

TimNN commented Jan 13, 2023

Mark-Simulacrum commented Jan 14, 2023

TimNN commented Jan 16, 2023 •

edited

Loading

TimNN commented Jan 16, 2023

Mark-Simulacrum commented Jan 21, 2023

TimNN commented Jan 21, 2023 •

edited

Loading

TimNN commented Jan 21, 2023 •

edited

Loading

Mark-Simulacrum commented Jan 21, 2023

bors commented Jan 21, 2023

bors commented Jan 21, 2023

bors commented Jan 21, 2023

rust-timer commented Jan 21, 2023

Remap paths in UI tests by default #105924

Remap paths in UI tests by default #105924

Conversation

TimNN commented Dec 19, 2022 • edited Loading

ChrisDenton commented Jan 6, 2023

TimNN commented Jan 6, 2023

TimNN commented Jan 6, 2023

jyn514 commented Jan 10, 2023 • edited Loading

TimNN commented Jan 13, 2023

Mark-Simulacrum commented Jan 14, 2023

TimNN commented Jan 16, 2023 • edited Loading

TimNN commented Jan 16, 2023

Mark-Simulacrum commented Jan 21, 2023

TimNN commented Jan 21, 2023 • edited Loading

TimNN commented Jan 21, 2023 • edited Loading

Mark-Simulacrum commented Jan 21, 2023

bors commented Jan 21, 2023

bors commented Jan 21, 2023

bors commented Jan 21, 2023

rust-timer commented Jan 21, 2023

Overall result: ✅ improvements - no action needed

Instruction count

Max RSS (memory usage)

Cycles

TimNN commented Dec 19, 2022 •

edited

Loading

jyn514 commented Jan 10, 2023 •

edited

Loading

TimNN commented Jan 16, 2023 •

edited

Loading

TimNN commented Jan 21, 2023 •

edited

Loading

TimNN commented Jan 21, 2023 •

edited

Loading